Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ECO-4688] Fix tests #391

Merged
merged 25 commits into from
May 30, 2024
Merged

[ECO-4688] Fix tests #391

merged 25 commits into from
May 30, 2024

Conversation

sacOO7
Copy link
Contributor

@sacOO7 sacOO7 commented Mar 7, 2024

@github-actions github-actions bot temporarily deployed to staging/pull/391/features March 7, 2024 19:31 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/391/docs March 7, 2024 19:32 Inactive
@sacOO7 sacOO7 changed the title Fix test execution Fix CI tests Mar 7, 2024
@sacOO7 sacOO7 changed the title Fix CI tests Fix tests Mar 7, 2024
@github-actions github-actions bot temporarily deployed to staging/pull/391/features April 22, 2024 14:42 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/391/docs April 22, 2024 14:43 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/391/features April 23, 2024 08:30 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/391/docs April 23, 2024 08:30 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/391/features April 23, 2024 10:55 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/391/docs April 23, 2024 10:55 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/391/features April 23, 2024 11:01 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/391/docs April 23, 2024 11:01 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/391/features April 23, 2024 11:02 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/391/docs April 23, 2024 11:03 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/391/features April 29, 2024 12:07 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/391/docs April 29, 2024 12:08 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/391/features April 29, 2024 16:06 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/391/docs April 29, 2024 16:06 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/391/features April 30, 2024 12:17 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/391/features May 16, 2024 05:45 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/391/docs May 16, 2024 05:45 Inactive
Copy link

@umair-ably umair-ably left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

couple formatting issues - happy for these to be fixed in your upcoming PR if that's easier

Copy link
Collaborator

@lawrence-forooghian lawrence-forooghian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've left some comments (and agree with Umair re all the unnecessary formatting changes).

I’d also like to mention that I found this PR difficult to review — in my opinion, unnecessarily so. There weren't any instructions on how to review it, so I tried reviewing it commit by commit. Unfortunately, there are many commits which turn out to be reverted by later commits, meaning that reviewing those commits turned out to be a waste of my time. Furthermore, the motivation for some of the changes is not very obvious, and isn't explained in the commit message, meaning that I had to spend time trying to understand what your intention was. I worry that if such a small PR was quite hard to review, then some of your upcoming larger protocol v2 ones will be very hard to review. Please could I ask that you spend time on those PRs to consider the experience for somebody trying to review them?

spec/unit/util/crypto_spec.rb Outdated Show resolved Hide resolved
spec/unit/util/crypto_spec.rb Outdated Show resolved Hide resolved
spec/unit/util/crypto_spec.rb Show resolved Hide resolved
spec/unit/util/crypto_spec.rb Show resolved Hide resolved
spec/acceptance/realtime/connection_spec.rb Outdated Show resolved Hide resolved
spec/unit/models/token_details_spec.rb Show resolved Hide resolved
lib/ably/realtime/channel.rb Outdated Show resolved Hide resolved
@sacOO7 sacOO7 changed the base branch from feature/no-connection-serial to main May 29, 2024 07:15
@github-actions github-actions bot temporarily deployed to staging/pull/391/features May 29, 2024 09:57 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/391/docs May 29, 2024 09:58 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/391/features May 29, 2024 10:06 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/391/docs May 29, 2024 10:07 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/391/features May 29, 2024 10:44 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/391/docs May 29, 2024 10:44 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/391/features May 29, 2024 12:14 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/391/docs May 29, 2024 12:14 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/391/features May 29, 2024 14:09 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/391/docs May 29, 2024 14:10 Inactive
@sacOO7
Copy link
Contributor Author

sacOO7 commented May 30, 2024

Thanks for approval 👍

@sacOO7 sacOO7 merged commit 062d98f into main May 30, 2024
22 of 23 checks passed
@sacOO7 sacOO7 deleted the fix/tests branch May 30, 2024 08:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

[ably-ruby] Fix failing tests
3 participants